Skip to content

Fix error on LAPACKE_*tpmqrt_work for ROW MAJOR matrices#540

Merged
langou merged 3 commits intoReference-LAPACK:masterfrom
weslleyspereira:fix-404-LAPACKE-dtpmqrt
Oct 18, 2022
Merged

Fix error on LAPACKE_*tpmqrt_work for ROW MAJOR matrices#540
langou merged 3 commits intoReference-LAPACK:masterfrom
weslleyspereira:fix-404-LAPACKE-dtpmqrt

Conversation

@weslleyspereira
Copy link
Collaborator

@weslleyspereira weslleyspereira commented Apr 22, 2021 •
edited
Loading

Fixes #404

Description
There are many bugs in LAPACKE_*tpmqrt_work routines for ROW MAJOR matrices, as pointed out by @haldaas in #404.

Checklist

  • Fix LAPACKE_dtpmqrt_work
  • Fix the remaining LAPACKE_*tpmqrt_work whenever my solution is validated for the double algorithm
  • Fix similar issues in other methods (if someone identifies it)

@weslleyspereira weslleyspereira mentioned this pull request Apr 22, 2021
Closed
@codecov
Copy link

codecov bot commented Apr 22, 2021 •
edited
Loading

Codecov Report

Base: 82.37% // Head: 82.37% // No change to project coverage 👍

Coverage data is based on head (1c33cda) compared to base (4b3c7c2).
Patch has no changes to coverable lines.

❗ Current head 1c33cda differs from pull request most recent head 64a0db9. Consider uploading reports for the commit 64a0db9 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #540   +/-   ##
=======================================
  Coverage   82.37%   82.37%           
=======================================
  Files        1894     1894           
  Lines      190681   190681           
=======================================
  Hits       157067   157067           
  Misses      33614    33614

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

angsch
angsch reviewed Oct 10, 2022
View reviewed changes
Copy link
Collaborator

@angsch angsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fixes of the leading dimensions look good. I like your solution to abstract from side by setting nrowsA, ncolsA and nrowsV.

@weslleyspereira
Copy link
Collaborator Author

weslleyspereira commented Oct 10, 2022

Thanks a lot for reviewing this PR, @angsch! I applied all your fixes in this last commit.

I will now apply the changes to the other LAPACKE_*tpmqrt_work.

angsch
angsch reviewed Oct 16, 2022
View reviewed changes
Copy link
Collaborator

@angsch angsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

The fix likely applies toLAPACKE_?tprfb_work, too. The dimensions of A depend on side. When side = 'Left', A has minimum dimensions (K, N). When side = 'Right', A is (M, K). The row major interface seems to only support side = 'Left':

lapack/LAPACKE/src/lapacke_ctprfb_work.c

Lines 52 to 53 in 28f7e83

} else if( matrix_layout == LAPACK_ROW_MAJOR ) {
lapack_int lda_t = MAX(1,k);

Perhaps it make sense to move this to a separate issue.

@langou langou merged commit e94df53 into Reference-LAPACK:master Oct 18, 2022
@julielangou julielangou added this to the LAPACK 3.11.0 milestone Nov 12, 2022
@martin-frbg martin-frbg mentioned this pull request Oct 12, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@angsch angsch angsch left review comments

@langou langou langou approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

LAPACK 3.11.0

Development

Successfully merging this pull request may close these issues.

LAPACKE_dtpmqrt_work wrong computation for row major layout

4 participants

@weslleyspereira @langou @angsch @julielangou